-
-
Notifications
You must be signed in to change notification settings - Fork 8.4k
frontend: Adjust source context bar #12039
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
862ba50
to
cf06745
Compare
cf06745
to
97e85a1
Compare
This has merge conflicts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Partial review. This has merge conflicts.
.arg(palette.color(QPalette::Window).name(QColor::HexRgb)) | ||
.arg(palette.color(QPalette::WindowText).name(QColor::HexRgb))); | ||
ui->colorPreview->setAutoFillBackground(true); | ||
//ui->colorPreview->setAlignment(Qt::AlignCenter); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't leave in commented out code.
#include <qt-wrappers.hpp> | ||
#include <OBSApp.hpp> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: these should be swapped.
/* force style sheet recalculation */ | ||
QString qss = widget->styleSheet(); | ||
widget->setStyleSheet("/* */"); | ||
widget->setStyleSheet(qss); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this no longer needed?
QString existingClasses = child->property("class").toString(); | ||
QStringList classList = existingClasses.split(" ", Qt::SkipEmptyParts); | ||
|
||
classList.removeDuplicates(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like there's an opportunity to create a private function for this since it's used multiple times. If you need both existingClasses
and classList
for later usage, you could return a std::pair<QString, QStringList>
.
addClass(ui->playPauseButton, "icon-media-pause"); | ||
removeClass(ui->playPauseButton, "icon-media-play"); | ||
removeClass(ui->playPauseButton, "icon-media-restart"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would setClasses(ui->playPauseButton, "icon-media-pause")
be more 1:1 here (and in the other Set*State
functions) instead of explicitly doing add
, remove
, remove
?
@@ -38,7 +38,6 @@ TextSourceToolbar::TextSourceToolbar(QWidget *parent, OBSSource source) | |||
const char *text = obs_data_get_string(settings, "text"); | |||
|
|||
bool single_line = !read_from_file && (!text || (strchr(text, '\n') == nullptr)); | |||
ui->emptySpace->setVisible(!single_line); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have before/after images for the TextSourceToolbar?
Description
Various tweaks and adjustments to the context bar appearance to bring it in line with the rest of the theme.
Motivation and Context
Want all aspects of the main window to look the same. Cleans up some UX quirks I was unhappy about.
The updated design follows @GeorgesStavracas' work on the obsproject/design repository. Some adjustments have been made after implementation and testing of the original design.
Original Mockup

Before

After

How Has This Been Tested?
👁
Types of changes
Checklist: